Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SmartPtr: Use shared ptr in RTC TCP connection. v6.0.127 #4083

Merged
merged 5 commits into from
Jun 13, 2024

Conversation

winlinvip
Copy link
Member

@winlinvip winlinvip commented Jun 12, 2024

Fix issue #3784

@winlinvip winlinvip added the EnglishNative This issue is conveyed exclusively in English. label Jun 12, 2024
trunk/src/app/srs_app_server.cpp Show resolved Hide resolved
trunk/src/app/srs_app_server.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@suzp1984 suzp1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix compiling error, when rtc is off.
./configure -rtc=off; make

trunk/src/app/srs_app_server.cpp Show resolved Hide resolved
Copy link
Contributor

@suzp1984 suzp1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wild pointer problem of SrsRtcTcpConn instance in conn_manager, the tcp connection resource manager.

@@ -1227,11 +1226,26 @@ srs_error_t SrsServer::do_on_tcp_client(ISrsListener* listener, srs_netfd_t& stf
}
}

// For RTC TCP connection, use resource executor to manage the resource.
SrsRtcTcpConn* raw_conn = dynamic_cast<SrsRtcTcpConn*>(resource);
if (raw_conn) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful the raw_conn (SrsRtcTcpConn resource) and its conn (SrsSharedResource wrapper), the raw_conn will be add to conn_manager, the tcp connection resource manager of SrsServer, in line 1244, and its wrapper, a SrsSharedResource, will created below in line 1233, this wrapper didn't added to any resource manager, but it will be released by _srs_rtc_manager in SrsExecutorCoroutine's destructor. Then the raw_conn stored in conn_manager will became a wild pointer, the good news is that the conn_manager will never remove it, so the process will never crash, but it's still a problem, is it?

So, there are two problem:

  1. the raw_conn will be added to conn_manager, which will never remove it;
  2. the raw_conn inside conn_manager will became wild pointer once its wapper, the SrsSharedResource released by _srs_rtc_manager inside SrsExecutorCoroutine's desctructor.

Copy link
Member Author

@winlinvip winlinvip Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nop, manager never adds raw_conn:

    // For RTC TCP connection, use resource executor to manage the resource.
    SrsRtcTcpConn* raw_conn = dynamic_cast<SrsRtcTcpConn*>(resource);
    if (raw_conn) {
        // ......
        return err;
    }

Won't fix this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get it.

@@ -436,19 +436,19 @@ srs_error_t SrsRtcUdpNetwork::write(void* buf, size_t size, ssize_t* nwrite)
return sendonly_skt_->sendto(buf, size, SRS_UTIME_NO_TIMEOUT);
}

SrsRtcTcpNetwork::SrsRtcTcpNetwork(SrsRtcConnection* conn, SrsEphemeralDelta* delta)
SrsRtcTcpNetwork::SrsRtcTcpNetwork(SrsRtcConnection* conn, SrsEphemeralDelta* delta) : owner_(new SrsRtcTcpConn())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current branch seems rebased on the #4084 now, which already supported constructor the SrsSharedResource instance from a NULL pointer, so I guess this initialization can be improved by:

Suggested change
SrsRtcTcpNetwork::SrsRtcTcpNetwork(SrsRtcConnection* conn, SrsEphemeralDelta* delta) : owner_(new SrsRtcTcpConn())
SrsRtcTcpNetwork::SrsRtcTcpNetwork(SrsRtcConnection* conn, SrsEphemeralDelta* delta) : owner_(NULL)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, reconsider this suggestion and let SrsSharedResource accept NULL pointer constructor, because

T* operator->() {
return ptr_.operator->();
}

in this case, it's owner_->interrupt().

@winlinvip winlinvip changed the title SmartPtr: Use shared ptr in RTC TCP connection. SmartPtr: Use shared ptr in RTC TCP connection. v6.0.127 Jun 13, 2024
@winlinvip winlinvip added the RefinedByAI Refined by AI/GPT. label Jun 13, 2024
@winlinvip winlinvip merged commit 242152b into ossrs:develop Jun 13, 2024
4 of 5 checks passed
winlinvip added a commit to winlinvip/srs that referenced this pull request Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EnglishNative This issue is conveyed exclusively in English. RefinedByAI Refined by AI/GPT.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants